security(dashboards): scope copied widgets to the copier's app access - #7858
Merged
Conversation
Dashboard permissions are intentionally separate from app permissions, so a
dashboard may be shared with a user who cannot read the apps its widgets point
at. Copying such a dashboard, however, made the copier the OWNER of the copied
widgets, and owners have edit rights. A view-only recipient could copy a shared
dashboard and then rewrite a borrowed widget's query - changing metrics,
visualization and breakdowns while leaving the original app id in place - to
read far more of that app than the dashboard owner chose to show them.
/o/dashboard/data and the /dashboard/data handlers serve whatever is in
widget.apps, so the rewritten widget returned data for an app the caller has no
read access to.
Copying a dashboard now skips any widget referencing an app the copier cannot
read. Widgets with no apps, such as notes, still copy.
Two further gaps in the same area:
- The existing app filter on add-widget/update-widget was guarded by
`widget.apps && Array.isArray(widget.apps)`, while isWidgetValid only checked
for `undefined`. An array-like object such as {length: 1, "0": "<app id>"}
therefore skipped the filter entirely, was stored as-is, and was still read
through .length/[i] by fetchWidgetApps and by every /dashboard/data handler.
This allowed injecting an arbitrary app id into a widget on a dashboard the
user owns outright, with no sharing or copying involved. Widget app lists are
now normalized to a real array of 24 character hex ids and rejected otherwise.
- add-widget silently stripped unauthorized app ids; it now rejects, so a widget
is never stored pointing at an app whose data will not be served.
Editing a widget on a dashboard shared with edit rights still keeps whatever
apps that widget already references - the dashboard owner delegated that by
granting edit access - but app ids being newly ADDED are checked against the
editor's own access.
Regression tests cover the reported copy chain, the array-like injection, adding
a widget for an unreadable app, and that copying still works for a user who can
read the apps.
Co-Authored-By: Claude <noreply@anthropic.com>
pnrgenc
approved these changes
Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reported via
security@count.ly. Assessed against master321a6645591e21c6e41f9834614d56126b780f45.Backport to
release.24.05: #7857 (that branch is the more exposed of the two — see below).The issue
Dashboard permissions are intentionally separate from app permissions, so a dashboard may be shared with a user who cannot read the apps its widgets reference. That part is by design.
Copying such a dashboard, however, made the copier the owner of the copied widgets, and owners have edit rights. A view-only recipient could copy a shared dashboard and then rewrite a borrowed widget's query — changing metrics, visualization and breakdowns while leaving the original app id in place — and read far more of that app than the dashboard owner chose to show them.
/o/dashboard/dataand the/dashboard/datahandlers serve whatever is inwidget.apps, so the rewritten widget returned data for an app the caller has no read access to.The sharer granted a view of one thing; ownership transfer turned it into an arbitrary query.
What changes
add-widgetrejects rather than strips. Previously unauthorized app ids were silently filtered out, which stored a widget pointing at an app whose data would never be served. It now returns 403.widget.apps && Array.isArray(widget.apps)whileisWidgetValidonly checked forundefined. A value that is array-like but not anArraytherefore skipped the filter entirely, was stored as-is, and was still read through.length/[i]byfetchWidgetAppsand by every/dashboard/datahandler — allowing an arbitrary app id on a dashboard the user owns outright, with no sharing or copying involved. App lists are now normalized to a real array of 24-character hex ids and rejected otherwise.update-widgetchecks only newly added apps. A member with edit access keeps whatever apps the widget already references, since the dashboard owner delegated that by sharing with edit rights. App ids being newly added are checked against the editor's own access. This required reading the stored widget before the update, since the frontend round-trips the full widget object — a blanket check would have rejected legitimate edits to delegated widgets.Why not enforce at read time instead
Filtering inside
fetchWidgetAppswas considered and rejected: it would break intentional cross-app dashboard sharing, which is a supported feature. It would also be insufficient on its own, because several/dashboard/datahandlers readwidget.appsdirectly rather than the resolved app map.Tests
plugins/dashboards/tests.jsgains four cases covering the copy chain, the array-like injection,add-widgetfor an unreadable app, and a positive case confirming copy still works for a user who can read the apps.Verified locally:
npx eslint plugins/dashboards/andnode --checkpass. The plugin suite runs in CI (test-api-plugins).Follow-ups, not in this PR
/i/dashboards/createresponse shape, which was kept out to keep this fix and its backport minimal.copy_dash_idline in the/i/dashboards/updateapidoc block is stale — that handler never reads it./report/authorizedispatch is commented out inplugins/reports/api/api.js, leaving/i/reports/createunauthorized forreport_type: "dashboards". Bounded by the render-time owner login, but worth its own ticket.getUserApps/getAdminApps(plain app membership) where alerts, hooks, reports and compare usegetUserAppsForFeaturePermission. Aligning it is a separate change with regression risk.🤖 Generated with Claude Code